Skip to content

Conversation

@szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Nov 19, 2025

Description

We added the live = 'rejectOnError' option in #2605 with the intention to make the behavior provided by setting live = 'rejectOnError' the default behavior of setting live = true in the next major.

Issues

Copy link
Member Author

szokeasaurusrex commented Nov 19, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@linear
Copy link

linear bot commented Nov 19, 2025

szokeasaurusrex added a commit that referenced this pull request Nov 19, 2025
### Description
We added the `live = 'rejectOnError'` option in #2605 with the intention to make the behavior provided by setting `live = 'rejectOnError'` the default behavior of setting `live = true` in the next major.

### Issues
- Resolves #2606
- Resolves [CLI-138](https://linear.app/getsentry/issue/CLI-138/v3-fix-js-interface-execute-promise-resolve-values)
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/no-live-rejectOnError branch from abf71f3 to 6b12318 Compare November 19, 2025 13:20
@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review November 19, 2025 13:22
@szokeasaurusrex szokeasaurusrex requested review from a team as code owners November 19, 2025 13:22
@szokeasaurusrex szokeasaurusrex requested review from AbhiPrasad and s1gr1d and removed request for a team, AbhiPrasad and s1gr1d November 19, 2025 13:22
szokeasaurusrex added a commit that referenced this pull request Nov 19, 2025
### Description
We added the `live = 'rejectOnError'` option in #2605 with the intention to make the behavior provided by setting `live = 'rejectOnError'` the default behavior of setting `live = true` in the next major.

### Issues
- Resolves #2606
- Resolves [CLI-138](https://linear.app/getsentry/issue/CLI-138/v3-fix-js-interface-execute-promise-resolve-values)
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/no-live-rejectOnError branch from 6b12318 to 40b77a1 Compare November 19, 2025 13:40
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! (cc @JPeer264 since we just talked about this)

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/no-live-rejectOnError branch from 40b77a1 to bd678cf Compare November 24, 2025 15:15
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/no-release-bundle-uploads branch from 3d01060 to 002c166 Compare November 24, 2025 15:15
// TODO (v3): Clean this up and always resolve a string (or change the type definition)
// @ts-expect-error - see comment above
resolve();
reject(new Error(`Command ${args.join(' ')} failed with exit code ${exitCode}`));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Missing else causes reject after successful resolve

The exit handler unconditionally calls reject() after conditionally calling resolve() for zero exit codes. When exitCode === 0, both resolve('success (live mode)') and reject(new Error(...)) execute sequentially. While promises only settle once, this creates an unhandled rejection and indicates incorrect control flow logic. An else clause or return statement is needed to prevent rejection after successful resolution.

Fix in Cursor Fix in Web

@szokeasaurusrex szokeasaurusrex added the v3.0 Breaking changes to include in version 3.0.0 of Sentry CLI label Nov 25, 2025
*
* @param {string} release Unique name of the release.
* @param {SentryCliUploadSourceMapsOptions & {live?: boolean | 'rejectOnError'}} options Options to configure the source map upload.
* @param {SentryCliUploadSourceMapsOptions} options Options to configure the source map upload.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szokeasaurusrex Here the live option is entirely removed, but in types.ts the live option is just adapted. What is now true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly, this removal is intentional. For sourcemaps upload, we always use live = true, and there is no way to override this (this is actually what we had prior to introducing the 'rejectOnError' option; we only made live configurable here to allow us to use 'rejectOnError').

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/no-release-bundle-uploads branch from 002c166 to 63f645f Compare December 3, 2025 09:24
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/no-live-rejectOnError branch from bd678cf to 2127f3c Compare December 3, 2025 09:24
### Description
We added the `live = 'rejectOnError'` option in #2605 with the intention to make the behavior provided by setting `live = 'rejectOnError'` the default behavior of setting `live = true` in the next major.

### Issues
- Resolves #2606
- Resolves [CLI-138](https://linear.app/getsentry/issue/CLI-138/v3-fix-js-interface-execute-promise-resolve-values)
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/no-live-rejectOnError branch from 2127f3c to d62403a Compare December 3, 2025 09:58
Comment on lines +335 to +338
if (exitCode === 0) {
resolve('success (live mode)');
}
// According to the type definition, resolving with void is not allowed.
// However, for backwards compatibility, we resolve void here to
// avoid a behaviour-breaking change.
// TODO (v3): Clean this up and always resolve a string (or change the type definition)
// @ts-expect-error - see comment above
resolve();
reject(new Error(`Command ${args.join(' ')} failed with exit code ${exitCode}`));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The pid.on('exit') handler unconditionally calls reject() after resolving when exitCode === 0, causing promises to always fail.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The pid.on('exit') handler in lib/helper.js lines 335-338 contains a logic error. When exitCode === 0, the promise correctly resolves with 'success (live mode)'. However, the reject() statement immediately follows this if block and is not enclosed in an else block, causing it to execute unconditionally. This means that even on successful process execution (exitCode === 0), the promise will first resolve and then immediately reject. This behavior causes Promise.all() calls, such as in uploadSourceMaps() in lib/releases/index.js, to fail entirely, as each individual promise will ultimately reject.

💡 Suggested Fix

Place the reject() statement within an else block for the if (exitCode === 0) condition in the pid.on('exit') handler to ensure rejection only occurs when exitCode !== 0.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: lib/helper.js#L335-L338

Potential issue: The `pid.on('exit')` handler in `lib/helper.js` lines 335-338 contains
a logic error. When `exitCode === 0`, the promise correctly resolves with `'success
(live mode)'`. However, the `reject()` statement immediately follows this `if` block and
is not enclosed in an `else` block, causing it to execute unconditionally. This means
that even on successful process execution (`exitCode === 0`), the promise will first
resolve and then immediately reject. This behavior causes `Promise.all()` calls, such as
in `uploadSourceMaps()` in `lib/releases/index.js`, to fail entirely, as each individual
promise will ultimately reject.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5120123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v3.0 Breaking changes to include in version 3.0.0 of Sentry CLI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants